Support loading dynamic libraries from multiple paths and try more paths in default loader #1864
Conversation
…ths in default loader
|
For reference, the list of paths is the same as used by SDL. This makes development on macos using the home-brew molten-vk package simpler since with this update the dylib can be found automatically (assuming paths are configured correctly). Otherwise you are forced to load the FunctionPointers yourself. |
| let get_proc_addr = { | ||
| let ptr: *mut c_void = dl | ||
| .symbol("vkGetInstanceProcAddr") | ||
| // Arguably we could continue trying subsequent paths here, but we don't | ||
| // expect to find a matching filename without the right contents, so better | ||
| // to fail here, especially since an unknown library is now loaded into the | ||
| // process. | ||
| .map_err(|_| { | ||
| LoadingError::MissingEntryPoint("vkGetInstanceProcAddr".to_owned()) | ||
| })?; | ||
| mem::transmute(ptr) | ||
| }; |
There was a problem hiding this comment.
DynamicLibrary seems to implement Drop, so would it be safe to continue attempting paths?
https://docs.rs/shared_library/0.1.9/shared_library/dynamic_library/struct.DynamicLibrary.html#impl-Drop
I don't know much about loading library, so I don't know what is safe and what isn't.
| get_proc_addr, | ||
| }) | ||
| } | ||
| None => Err(LoadingError::LibraryLoadFailure(errors.join(" "))), |
There was a problem hiding this comment.
I not sure I am a huge fan of the space separated errors. shared_library isn't very helpful with just returning a String. Perhaps the errors should be a Vec<LoadingError { path: Path, error: String }>?
|
Vulkano uses Ash, and Ash has its own loading implementation. Perhaps these changes would be useful for them as well? I've been considering making Vulkano use Ash's loading code so then Vulkano would also benefit. |
Eliah-Lakhin
added
the
PR: In review. Comments to address.
Change the DynamicLibraryLoader::new interface to accept a slice of paths
instead of a single path. It tries to load them in order, using the first one it is
able to load. Update the auto_loader implementation to use the updated
version, including a larger set of paths for the Vulkan library.
Describe in common words what is the purpose of this change, related
Github Issues, and highlight important implementation aspects.
Please put changelog entries in the description of this Pull Request
if knowledge of this change could be valuable to users. No need to put the
entries to the changelog directly, they will be transferred to the changelog
files(
CHANGELOG_VULKANO.mdandCHANGELOG_VK_SYS.md)by maintainers right after the Pull Request merge.
Entries for Vulkano changelog:
**Breaking** Breaking entry description.Non-breaking entry description....Entries for VkSys changelog:
Entry 1.Entry 2....Run
cargo fmton the changes.Make sure that the changes are covered by unit-tests.
Update documentation to reflect any user-facing changes - in this repository.